Move get_file and save_file to ConfigurationManagementMixin #17494
Conversation
|
@carbonin While I left the yaml interface here in this PR, I'm wondering if instead that should just live in the advanced_settings screen directly, essentially moving the yaml handling code and the password encryption/decryption there. Thoughts? |
|
Not changing the YAML.load calls as rubocop wants. |
|
@Fryguy tested it with ManageIQ/manageiq-ui-classic#3967, seems to work 👍 |
Fryguy
force-pushed
the
move_get_file_set_file
branch
2 times, most recently
from
73e383a to
a3ada01
Compare
| hash = | ||
| begin | ||
| decrypt_passwords!(YAML.load(contents)) | ||
| rescue => err |
There was a problem hiding this comment.
If you look at the previous code, this enumerated both StandardError and Psych::SyntaxError. We had done this in the past because Psych::SyntaxError used to not derive from StandardError, but in Ruby 2.3 and 2.4 that is no longer the case, so we can just catch StandardError like normal. I did this because rubocop rightly complains with Lint/ShadowedException
[1] pry(main)> Psych::SyntaxError < StandardError
=> true
[2] pry(main)> RUBY_VERSION
=> "2.3.3"
[3] pry(main)> Psych::VERSION
=> "2.1.0"
| # | ||
| # As an example, ntp_reload works by telling systemctl to restart | ||
| # chronyd. However, if this occurs on every worker, you end up with | ||
| # dozens of calls to `systemctl chrony restart` simultaneously. |
Fryguy
force-pushed
the
move_get_file_set_file
branch
from
a3ada01 to
f15efd3
Compare
|
@carbonin Fixed. Also, I added an extra spec for the different types of YAML failures, and there's a note above about the changes to how it's rescued (not sure you saw those changes in your review) |
|
I am not fixing the Style/RescueStandardError. I just disagree with it and am going to open a PR to guides to change the default. |
| # | ||
| # As an example, ntp_reload works by telling systemctl to restart | ||
| # chronyd. However, if this occurs on every worker, you end up with | ||
| # dozens of calls to `systemctl chronyd restart` simultaneously. |
There was a problem hiding this comment.
Still... it's systemctl restart chronyd
In moving, the methods become settings_for_resource_yaml and add_settings_for_resource_yaml, respectively. The old interfaces are still left intact temporarily until the UI is changed to use the new methods. Also preserved is the special activation that is currently done for ntp_reload, as introduced in ManageIQ/manageiq@939524fa, which avoids repeated activation for every worker, instead preferring to do it once at the server level. https://bugzilla.redhat.com/show_bug.cgi?id=1082155
Fryguy
force-pushed
the
move_get_file_set_file
branch
from
f15efd3 to
46fae81
Compare
|
Checked commit Fryguy@46fae81 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 lib/vmdb/settings.rb
spec/lib/vmdb/settings_spec.rb
|
|
@carbonin Done for real now. Also, thoughts on #17494 (comment) ? |
I guess it depends on if you want that to be a "supported API" or if that's just a quirk of the way we are displaying things. I'm fine with either, but I think it only makes sense if we admit that saving the whole YAML at once is an exception rather than the norm. |
In moving, the methods become settings_for_resource_yaml and
add_settings_for_resource_yaml, respectively. The old interfaces are
still left intact temporarily until the UI is changed to use the new
methods.
Also preserved is the special activation that is currently done for
ntp_reload, as introduced in 939524fa, which avoids
repeated activation for every worker, instead preferring to do it once
at the server level.
Replaces #17458
@carbonin Please review
cc @h-kataria